[fix][ml] Cancel Metadata Store listeners at shutdown to fix resource leaks in some tests #24256
Conversation
… leaks in some tests
...ar-metadata/src/main/java/org/apache/pulsar/metadata/api/extended/MetadataStoreExtended.java
Show resolved
Hide resolved
…registerSessionListener"
|
@lhotari This test has failed five times, maybe you should look into it. |
| * Executing this Runnable will cancel the subscription. | ||
| */ | ||
| void registerListener(Consumer<Notification> listener); | ||
| Runnable registerCancellableListener(Consumer<Notification> listener); |
There was a problem hiding this comment.
@lhotari It looks like this change breaks compatibility with existing implementations of registerListener.
Currently, you always invoke registerCancellableListener, which causes a compile-time failure if the custom metadata store only implements the older registerListener method.
To maintain backward compatibility, registerCancellableListener should delegate to registerListener by default and return a noop runnable. For example:
default Runnable registerCancellableListener(Consumer<Notification> listener) {
registerListener(listener);
return () -> {
// noop
};
}
@Deprecated
default void registerListener(Consumer<Notification> listener) {
// noop
}
This ensures compatibility while allowing newer implementations to override registerCancellableListener directly.
There was a problem hiding this comment.
If you use this way, please add a testcase to cover this change.
There was a problem hiding this comment.
It looks like this change breaks compatibility with existing implementations of registerListener.
Currently, you always invoke registerCancellableListener, which causes a compile-time failure if the custom metadata store only implements the older registerListener method.
@nodece That's correct. Binary compatibility would be broken for implementors of the API. Implementors should recompile unless they are already using AbstractBatchedMetadataStore or AbstractMetadataStore base classes. The problem in making both the registerCancellableListener and registerListener a default method is that the compiler won't assist an implementor to implement the method that isn't deprecated.
I'm not aware of implementations that aren't using AbstractBatchedMetadataStore or AbstractMetadataStore base class. The number of implementors is unknown, but I'd assume that the ones that are capable of doing so are also capable of recompiling when upgrading.
7 participants
Motivation
The current MetadataStore API and MetadataStoreExtended API doesn't provide a way to cancel (unregister) registered listeners. This is a problem in some tests where the metadata store instance is shared, but the brokers are restarted during the test. Notifications will keep on delivered to the broker instances that have already been shutdown.
In addition, to the above problem, the listeners will also keep a strong reference to instances that have already been shutdown, impacting memory usage in some tests.
One of the additional motivations is to cleanup the testing infrastructure so that it would be possible to migrate Pulsar to use the most recent Mockito version, 5.17.0. When mocks are invalidated in newer Mockito versions, they will throw exceptions upon execution. This change is made to ensure that these problems that are currently seen in PR #24241 aren't originating from Metadata Store listeners.
Modifications
Add new methods:
registerCancellableListenerto MetadataStoreregisterCancellableSessionListenerto MetadataStoreExtendedMake the API binary compatible by adding default methods
registerListenerandregisterSessionListenerwith the previous method signatures so that plugins using the Metadata API won't break due to this change. Mark these methods deprecated.Migrate the Pulsar code to use the cancellable methods and store the callback instance for cancellation and use this in the shutdown sequence of each component that registers listeners.
Documentation
docdoc-requireddoc-not-neededdoc-complete